Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I9992 #366

Merged
merged 24 commits into from
Jul 2, 2024
Merged

I9992 #366

merged 24 commits into from
Jul 2, 2024

Conversation

jardakotesovec
Copy link
Collaborator

No description provided.

…ure that the form errors are propagated to the modal for other ListPanels
…some other modals for more consistent naming
v-if="currentAttacher"
v-bind="currentAttacher"
@selected:files="(...args) => emit('attachFiles', ...args)"
@cancel="(...args) => emit('cancel', ...args)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI: my IDE was labeling the @cancel event as deprecated, but I couldn't find anything more about it.

<td>{{ dateRangeDescription }}</td>
</tr>
</table>
<action-panel class="pkpStats__reportAction">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed here <action-panel> and <pkp-button> are used whereas other places (I noticed in PublicationsDowloadReportModal but wasn't comprehensive in checking everywhere) <PkpButton> and <ActionPanel> are used. I know both will work, but wondering if we've standardized on a "house style" for this and if so, if it could be encapsulated in an eslint rule or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ewhanson I am almost always aiming for vue defaults. Which in case is to use ActionPanel in vue SFC files and only to use action-panel when its defined in 'html', which in our cases would be in smarty templates that we are moving away from. Having eslint rule for this is good idea, will check that out.

<template>
<p>{{ t('manager.dois.update.partialFailure') }}</p>
<ul>
<li v-for="errorMessage in failedDoiActions" :key="errorMessage.index">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The <li> items have list-style: none; applied to them (which wasn't the case previously).

DOI failed updates in OJS 3.4

From OJS 3.4 ☝️

DOI failed updates modal in this PR

From this PR ☝️

</div>

<div class="doiListItem__versionContainer--actionsBar">
<spinner v-if="isSaving" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be working as intended, but neither <spinner> nor <pkp-button> on the next line have been imported. Not sure if it's an issue, but wanted to flag it for you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ewhanson Thanks - its because most component are registered globally. But I think still there is value to import dependencies always so its clear on which components it depends.. for example in storybook its important.

I wish this would be caught by tooling like eslint as well.. maybe its possible, will check.

@@ -421,3 +423,4 @@ export default {
}
}
</style>
./HighlightsEditModal.vue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this probably isn't supposed to be here.

<template>
<SideModalBody>
<template #title>
{{ t('submission.wizard.changeSubmission') }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes use of t but doesn't have the corresponding const {t} = useLocalize(); in the script below.


## Usage

We recommend to define modals as individual component files, rather than inline them, that ensures that the `setup` function and any other component life cycle event are triggered as modal is opened/closed. Therefore its easier to control when for example to fetch data from API. We might introduce option to define inline modals for basic use cases in future
Its important to create modals as separate files, which works best with our [useModal](../?path=/docs/composables-usemodal--docs#opensidemodal) composable. Also having them defined as individual components ensures that the `setup` function and any other component life cycle event are triggered as modal is opened/closed. Therefore its easier to control when for example to fetch data from API.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both its should be It's. Therefore it's easier could also be Therefore, it's easier, but that's more of a style preference.

# useModal

## openDialog

Dialogs provide a quick way to show a simple confirmation prompt. Import `useModal` composable and use the openDialog() method to create a dialog.
Dialog purpose is to display simple feedback like success and error messages. Or request confirmation for example before deleting some content. Import `useModal` composable and use the openDialog() method to create a dialog. Check [Dialog](../?path=/docs/components-dialog--docs) component for more details on available props.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another minor wording suggestion: instead of Dialog purpose, maybe The Dialog component's purpose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, wording suggestions are much appreciated, not my strong suite :-)

@jardakotesovec jardakotesovec merged commit 76ca2af into pkp:main Jul 2, 2024
2 checks passed
@jardakotesovec jardakotesovec deleted the i9992 branch July 2, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants